Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

For #6687: Add crash reporting information for ClassCastException in PromptFeature #6995

Merged
merged 1 commit into from
May 25, 2020

Conversation

rocketsroger
Copy link
Contributor


Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@rocketsroger rocketsroger added the 🕵️‍♀️ needs review PRs that need to be reviewed label May 15, 2020
@rocketsroger rocketsroger requested a review from pocmo May 15, 2020 17:17
@codecov
Copy link

codecov bot commented May 15, 2020

Codecov Report

Merging #6995 into master will increase coverage by 0.22%.
The diff coverage is 64.51%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #6995      +/-   ##
============================================
+ Coverage     76.85%   77.07%   +0.22%     
- Complexity     4739     4832      +93     
============================================
  Files           647      642       -5     
  Lines         23622    23645      +23     
  Branches       3453     3446       -7     
============================================
+ Hits          18154    18224      +70     
+ Misses         4020     3970      -50     
- Partials       1448     1451       +3     
Impacted Files Coverage Δ Complexity Δ
...ozilla/components/feature/prompts/PromptFeature.kt 79.09% <64.51%> (+0.34%) 47.00 <0.00> (ø)
...components/support/sync/telemetry/SyncTelemetry.kt 85.71% <0.00%> (-1.39%) 41.00% <0.00%> (ø%)
...a/components/support/migration/TelemetryHelpers.kt 90.78% <0.00%> (-0.22%) 0.00% <0.00%> (ø%)
...onents/support/sync/telemetry/BaseGleanSyncPing.kt 100.00% <0.00%> (ø) 11.00% <0.00%> (ø%)
...ents/ui/autocomplete/InlineAutocompleteEditText.kt
...components/feature/accounts/push/SendTabFeature.kt
...omponents/feature/accounts/push/SendTabUseCases.kt
...ozilla/components/support/android/test/Matchers.kt
...port/android/test/espresso/matcher/ViewMatchers.kt
...nts/feature/accounts/push/FxaPushSupportFeature.kt
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d760644...9fd251c. Read the comment docs.

Breadcrumb.Level.DEBUG, Breadcrumb.Type.NAVIGATION)
)

crashReporting?.submitCaughtException(e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really like the idea of catching ClassCastException here every time since it will just hide errors while developing. Maybe we could just consider adding regular breadcrumbs to the feature (showing prompt, confirming, cancelling) that are helpful for understanding all kinds of errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll add breadcrumb and throw the exception after.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I was thinking about just generic breadcrumbs independent of the exception here.

But talking about the exception. Maybe you do not want a breadcrumb and instead just rethrow a new exception with a better message (and the original one as cause)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am worried this is called too many times to add a generic breadcrumb without an exception. Sure, I could modify the message. But wouldn't that be similar to adding a breadcrumb since we will be try catching the exception and rethrowing?

@rocketsroger rocketsroger force-pushed the AC_6687 branch 2 times, most recently from 45c17f8 to 01b9aac Compare May 22, 2020 22:16
@rocketsroger rocketsroger requested a review from pocmo May 25, 2020 14:03
@rocketsroger
Copy link
Contributor Author

bors r=pocmo

@bors
Copy link

bors bot commented May 25, 2020

Build succeeded:

@bors bors bot merged commit f32794f into mozilla-mobile:master May 25, 2020
@rocketsroger rocketsroger deleted the AC_6687 branch May 25, 2020 16:38
@rocketsroger rocketsroger removed the 🕵️‍♀️ needs review PRs that need to be reviewed label Jun 3, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants